Fix #669: Prevent dangling ConnectionState from GetOffer messages #680
Conversation
…messages - Modified handle_message_taproot to return (Response, bool) tuple - GetOffer now creates temporary state that is never persisted - SwapDetails and subsequent messages persist state normally - Handles both cases: GetOffer called or skipped
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix issue #669 by preventing dangling ConnectionState entries from GetOffer messages. However, the implementation introduces a critical bug that breaks the normal swap flow.
Key Changes:
- Modified
handle_message_taprootto return a tuple(Response, bool)where the boolean indicates whether state should be persisted - GetOffer now uses temporary state that is never persisted
- SwapDetails and subsequent messages persist state normally
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/maker/handlers2.rs | Updated function signature to return persistence flag; modified SwapDetails to generate keypair if missing; added documentation for new return type |
| src/maker/server2.rs | Changed connection state handling to use temporary state for GetOffer; added conditional persistence logic based on should_persist flag; updated error messages |
Critical Issue Found:
The PR breaks the standard swap flow where a taker sends GetOffer followed by SwapDetails. When GetOffer is received, the maker generates a keypair and sends the public key to the taker, but this state is not persisted. When SwapDetails arrives, the maker generates a NEW keypair, causing a mismatch. The taker will use the wrong public key when constructing contracts, causing the swap to fail. This is evidenced in the taker code at src/taker/api2.rs:732 where the taker stores and uses the tweakable_point from the GetOffer response.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@stark-3k would you please review this !! |
…messages - Modified handle_message_taproot to return (Response, bool) tuple - GetOffer now creates temporary state that is never persisted - SwapDetails and subsequent messages persist state normally - Handles both cases: GetOffer called or skipped
|
@omsuneri we don't allow AI-generated code in this project. The code we are writing is security-critical and requires a lot of care while changing core modules like this. Please refrain from opening PRs if you gonna write AI slops. We won't spend time reviewing this. Consider this a warning: if you put more AI-generated PRs like this, you can get banned from the project. Write your own code, or don't bother writing at all. |
|
@mojoX911 i did it on my own but dunno how the ai review turns on in my pr and i m getting the review automatically written |
|
You are changing documentations and other code that are not related to the task at all. |
|
@mojoX911 ohhh actually the changes made the functionality change so i modified the comments if you want i can just add them |
|
The changeset is still too noisy to review. I don't know what's going on. Its definitely changing unrelated code lines. Not possible to micro-review each change. Like random logs being changed etc. Try doing the refactor in only the specific required location without changing anything else. Also this is not in priority, as the team is busy with the v2 release. We will be picking up reviews on current open issues from next weeks. |
|
should i close this PR |
|
You can try for yourself as long as you want. Try to follow these points.
|
sure, will keep this in mind from ahead !! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #680 +/- ##
==========================================
+ Coverage 68.87% 77.53% +8.66%
==========================================
Files 35 49 +14
Lines 4932 15007 +10075
==========================================
+ Hits 3397 11636 +8239
- Misses 1535 3371 +1836 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Closing this as it will be handled separately. |
2 participants
fixes #669